Deprecate use of v1 tasks within v2 workflows #1721

tcompa · 2024-09-03T08:28:04Z

This cannot be merged&released until we verify that production instances do not include any such task (see script in #1712).

closes #1722

Checklist before merging

Update Fix migration for sqlite #1722
Test migration failure when there was still some legacy task in v2 workflows
I checked all FIXME 1712 entries
I added an appropriate entry to CHANGELOG.md
I added logging to new code - if appropriate.
I merged main into the current branch.

tcompa · 2024-09-03T08:29:50Z

Note that the CI currently fails because of sqlite:

NotImplementedError: No support for ALTER of constraints in SQLite dialect. Please refer to the batch mode feature which allows for SQLite migrations using a copy-and-move strategy.

github-actions · 2024-09-03T08:32:23Z

Benchmark comparison

GET /api/alive/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`
vanilla	1.1	0.8	1.38
power	0.9	0.8	1.12
dataset	0.8	0.8	1.00
project	1.0	0.8	1.25
job	0.8	0.8	1.00

GET /api/v2/dataset/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`	Size current (Kb)	Size `main` (Kb)
vanilla	9.7	9.6	1.01	0.6	0.7
power	410.8	420.2	0.98	389.0	440.2
dataset	164.1	165.6	0.99	145.6	171.2
project	49.3	50.8	0.97	50.9	58.9
job	27.8	30.2	0.92	35.3	41.7

GET /api/v2/job/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`	Size current (Kb)	Size `main` (Kb)
vanilla	6.0	5.7	1.05	2.2	2.2
power	141.6	134.7	1.05	652.0	652.0
dataset	151.4	140.5	1.08	904.8	904.8
project	49.0	45.9	1.07	283.7	283.7
job	41.5	40.0	1.04	224.8	224.8

GET /api/v2/project/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`	Size current (Kb)	Size `main` (Kb)
vanilla	9.0	6.1	1.48	0.1	0.1
power	9.6	5.6	1.71	0.1	0.1
dataset	8.8	6.0	1.47	0.1	0.1
project	12.6	8.3	1.52	2.3	2.3
job	9.3	5.9	1.58	0.1	0.1

GET /api/v2/workflow/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`	Size current (Kb)	Size `main` (Kb)
vanilla	19.0	10.8	1.76	0.8	0.8
power	30.2	24.8	1.22	27.2	29.8
dataset	21.3	18.8	1.13	15.8	17.1
project	23.1	22.7	1.02	19.9	21.5
job	12.6	10.2	1.24	0.8	0.9

POST /api/v2/project/3/dataset/403/images/query/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`	Size current (Kb)	Size `main` (Kb)
dataset	160.5	146.7	1.09	275.1	275.1

GET /auth/current-user/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`	Size current (Kb)	Size `main` (Kb)
vanilla	4.9	4.3	1.14	0.2	0.2
power	4.5	4.2	1.07	0.2	0.2
dataset	4.2	4.1	1.02	0.2	0.2
project	4.2	4.1	1.02	0.2	0.2
job	4.2	4.1	1.02	0.2	0.2

POST /auth/token/login/

User	Time current (ms)	Time `main` (ms)	Ratio current/`main`	Size current (Kb)	Size `main` (Kb)
dataset	262.5	258.3	1.02	0.2	0.2

…n-v2-workflows

…ows' of github.com:fractal-analytics-platform/fractal-server into 1712-on-hold-deprecate-use-of-v1-tasks-within-v2-workflows

…n-v2-workflows

…ropping column

tcompa · 2024-09-09T13:53:26Z

For my understanding: Do we need to remove V1 legacy tasks from Fractal server or can we still have them there in old workflows, just without the possibility of running them?

The first one you said: we cannot have V1 tasks linked to V2 tasks any more. Because this action is required, we are completing this PR but we will not merge&release it until the relevant instances are ready.

If we go for an option where we need to first remove them from all workflows, I think it's doable. There are some 30-40 workflows on the Liberali server with V1 legacy tasks, we could manually replace them with the V2 version of the task in each workflow (e.g. download the json parameters of the task, add the new version, upload the parameters there)

Yes, this would the preferred approach. The script from #1712 should let you identify the workflows quickly, but then the legacy-task replacement has to be manual. We can also prepare a script for performing this kind of replacements, if that is useful, although it's TBD whether it would actually save us time.

jluethi · 2024-09-09T15:38:20Z

Yes, this would the preferred approach. The script from #1712 should let you identify the workflows quickly, but then the legacy-task replacement has to be manual. We can also prepare a script for performing this kind of replacements, if that is useful, although it's TBD whether it would actually save us time.

I think we'll be able to do this manually within an hour or 2, so no need for a script. It's not optimal that we have to change old workflows, but I agree that the trade-off here is worth it.

I'll do this at FMI & can also handle it at UZH once the Ilastik task has been tested & verified :)

jluethi · 2024-09-09T16:29:27Z

All legacy tasks in V2 workflows have now been removed from both servers :)

…n-v2-workflows

ychiucco · 2024-09-10T12:19:47Z

The migration added by this PR will fail (here) if performed on a database with a legacy Task in a WorkflowV2.

Test:

Switch to main and create a DB such that

select is_legacy_task from workflowtaskv2;

is_legacy_task
----------------
 f
 t
(2 rows)

Switch to 1712-on-hold-deprecate-use-of-v1-tasks-within-v2-workflows and run fractalctl set-db:

WARNING:root:FRACTAL_TASKS_DIR="tasks" is not an absolute path; converting it to "/Users/yuri/Desktop/eXact/fractal/fractal-server/tasks"
WARNING:root:FRACTAL_RUNNER_WORKING_BASE_DIR="artifacts" is not an absolute path; converting it to "/Users/yuri/Desktop/eXact/fractal/fractal-server/artifacts"
START: Run alembic.config, with argv=['-c', '/Users/yuri/Desktop/eXact/fractal/fractal-server/fractal_server/alembic.ini', 'upgrade', 'head']
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 091b01f51f88 -> d9a140db5d42, Remove link between v1 and v2 tasks/workflowtasks tables
Traceback (most recent call last):
...
...
asyncpg.exceptions.NotNullViolationError: column "task_id" of relation "workflowtaskv2" contains null values

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
...
...

sqlalchemy.exc.IntegrityError: (sqlalchemy.dialects.postgresql.asyncpg.IntegrityError) <class 'asyncpg.exceptions.NotNullViolationError'>: column "task_id" of relation "workflowtaskv2" contains null values
[SQL: ALTER TABLE workflowtaskv2 ALTER COLUMN task_id SET NOT NULL]
(Background on this error at: https://sqlalche.me/e/20/gkpj)

Remove the legacy Task from the WorkflowV2, i.e.

select is_legacy_task from workflowtaskv2;

is_legacy_task
----------------
 f
(1 row)

Run again fractalctl set-db:

WARNING:root:FRACTAL_TASKS_DIR="tasks" is not an absolute path; converting it to "/Users/yuri/Desktop/eXact/fractal/fractal-server/tasks"
WARNING:root:FRACTAL_RUNNER_WORKING_BASE_DIR="artifacts" is not an absolute path; converting it to "/Users/yuri/Desktop/eXact/fractal/fractal-server/artifacts"
START: Run alembic.config, with argv=['-c', '/Users/yuri/Desktop/eXact/fractal/fractal-server/fractal_server/alembic.ini', 'upgrade', 'head']
INFO  [alembic.runtime.migration] Context impl PostgresqlImpl.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 091b01f51f88 -> d9a140db5d42, Remove link between v1 and v2 tasks/workflowtasks tables
END: alembic.config

START: Default group creation
END: Default group creation

START: First user creation
END: First user creation

…n-v2-workflows

tcompa added 7 commits September 3, 2024 10:10

Deprecate legacy tasks in V2 workflows - first commit

b530867

Add db migration script

4893927

Update scripts/validate_db_data_with_read_schemas.py

643ca87

Update benchmarks/runner/mocks.py

8e27eef

Do not test order of listdir output

218ff12

Add FIXMEs

0053a38

Adapt tests after removing legacy tasks from V2 workflows

9601cfe

tcompa linked an issue Sep 3, 2024 that may be closed by this pull request

Deprecate use of V1 tasks within V2 workflows #1712

Closed

tcompa added 2 commits September 3, 2024 10:35

Make non-legacy workflowtaskv2 task attributes non-optional

f8b7ae4

Update db migration script

2e0588f

tcompa mentioned this pull request Sep 3, 2024

Deprecate support for legacy tasks in V2 workflows fractal-analytics-platform/fractal-web#545

Closed

tcompa and others added 17 commits September 3, 2024 11:47

Remove Task.is_v2_compatible and task-legacy endpoints

b31d9d2

Update tests

4afa118

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

e2da880

…n-v2-workflows

Keep removing legacy-task-related tests

d9e97b4

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

6c88926

…n-v2-workflows

Add required task_id attribute in tests

9df9c85

Update test_unit_schemas_v2.py

a4eebf1

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

9ec7b6d

…n-v2-workflows

Fix some more WorkflowTaskV2Mock required attributes in tests

216949d

Merge branch '1712-on-hold-deprecate-use-of-v1-tasks-within-v2-workfl…

5901a2e

…ows' of github.com:fractal-analytics-platform/fractal-server into 1712-on-hold-deprecate-use-of-v1-tasks-within-v2-workflows

Merge two migration scripts

650df6a

Make migration script work for sqlite (ref #1722)

a9644d5

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

14ac70b

…n-v2-workflows

Change sqlite-motivated order of migration script statements

5efb607

Do not explicitly drop foreign-key constraint, since we are already d…

c8b38d8

…ropping column

Change sqlite-motivated order of migration script statements

da6443c

Add info about current sqlite version to warning message

1edff7b

ychiucco added 4 commits September 9, 2024 16:07

add one missing piece to migration

978429b

remove index

2f16169

if branch

de47a46

check for index

146e31c

ychiucco and others added 8 commits September 10, 2024 09:20

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

0c6e25d

…n-v2-workflows

fix revision order

8330543

Literal for WorkflowTaskV2.task_type

a3f0690

rollback Literal

e233355

Update logs

80022a6

Update migration name

e680495

Update remove_link_between_v1_and_v2_tasks migration script for sqlite

82f395a

Re-order migration statements

0095026

ychiucco approved these changes Sep 10, 2024

View reviewed changes

tcompa added 6 commits September 10, 2024 16:03

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

41fbc2c

…n-v2-workflows

Clean up old explanation comment

9a9d2b6

Update CHANGELOG [skip ci]

e28d9ef

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

b1acbef

…n-v2-workflows

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

32a1045

…n-v2-workflows

Merge branch 'main' into 1712-on-hold-deprecate-use-of-v1-tasks-withi…

7c81fbd

…n-v2-workflows

tcompa changed the title ~~[DO NOT MERGE] Deprecate use of v1 tasks within v2 workflows~~ Deprecate use of v1 tasks within v2 workflows Sep 13, 2024

tcompa marked this pull request as ready for review September 13, 2024 11:26

tcompa added 2 commits September 13, 2024 13:28

Update CHANGELOG.md [skip ci]

d973896

Update __main__.py

e49247d

tcompa merged commit f365226 into main Sep 13, 2024
20 checks passed

tcompa deleted the 1712-on-hold-deprecate-use-of-v1-tasks-within-v2-workflows branch September 13, 2024 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate use of v1 tasks within v2 workflows #1721

Deprecate use of v1 tasks within v2 workflows #1721

tcompa commented Sep 3, 2024 •

edited

Loading

tcompa commented Sep 3, 2024

github-actions bot commented Sep 3, 2024 •

edited

Loading

tcompa commented Sep 9, 2024

jluethi commented Sep 9, 2024

jluethi commented Sep 9, 2024

ychiucco commented Sep 10, 2024

Deprecate use of v1 tasks within v2 workflows #1721

Deprecate use of v1 tasks within v2 workflows #1721

Conversation

tcompa commented Sep 3, 2024 • edited Loading

Checklist before merging

tcompa commented Sep 3, 2024

github-actions bot commented Sep 3, 2024 • edited Loading

Benchmark comparison

GET /api/alive/

GET /api/v2/dataset/

GET /api/v2/job/

GET /api/v2/project/

GET /api/v2/workflow/

POST /api/v2/project/3/dataset/403/images/query/

GET /auth/current-user/

POST /auth/token/login/

tcompa commented Sep 9, 2024

jluethi commented Sep 9, 2024

jluethi commented Sep 9, 2024

ychiucco commented Sep 10, 2024

tcompa commented Sep 3, 2024 •

edited

Loading

github-actions bot commented Sep 3, 2024 •

edited

Loading